Skip to content

Avoid generating properties based on synthetic bridge methods #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Avoid generating properties based on synthetic bridge methods #106

wants to merge 3 commits into from

Conversation

robbytx
Copy link
Contributor

@robbytx robbytx commented Sep 17, 2020 •
edited
Loading

Problem

Java 5 supports covariant return types, which if overridden cause the compiler to generate "bridge" methods: https://stackoverflow.com/questions/14694852/can-overridden-methods-differ-in-return-type

These methods appear in Class.getDeclaredMethods() and if they happen to appear before the true non-bridge method, then they can cause BeanBuilder either to:

  1. Generate a getter method with the wrong return type, or
  2. Skip generation of a necessary getter method due to seeing the "concrete" bridge method.

On my machine (Java 1.8.0_181-b13 on Mac), the failing test cases produce two distinct errors:

  1. Jackson data-binding failure because MrBean generated a setter with the parent type.
com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized property "flavor" (class com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$Drink), not marked as ignorable (one known property: "type"])
 at [Source: (String)"{"drink":{"type":"coffee","flavor":"pumpkin spice"}}"; line: 1, column: 37] (through reference chain: com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$CoffeeHolder["drink"]->com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$Drink["flavor"])

	at com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException.from(UnrecognizedPropertyException.java:52)
	at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownProperty(DeserializationContext.java:1061)
	at com.fasterxml.jackson.databind.deser.std.StdDeserializer.handleUnknownProperty(StdDeserializer.java:1606)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownProperty(BeanDeserializerBase.java:1606)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1584)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._vanillaDeserializeWithUnknown(BeanDeserializer.java:443)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._vanillaDeserialize(BeanDeserializer.java:382)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:205)
	at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._vanillaDeserialize(BeanDeserializer.java:338)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:205)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:269)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:2420)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:1431)
	at com.fasterxml.jackson.module.mrbean.TestBridgeMethods.testSimpleCovariantProperty(TestBridgeMethods.java:59)
  1. AbstractMethodError because MrBean skipped generating a getter due to the bridge method:
java.lang.AbstractMethodError: Method com/fasterxml/jackson/module/mrbean/generated/com/fasterxml/jackson/module/mrbean/TestBridgeMethods$SpecificCoffeeHolder.getObject()Lcom/fasterxml/jackson/module/mrbean/TestBridgeMethods$Coffee; is abstract

	at com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$SpecificCoffeeHolder.getObject(Unknown Source)
	at com.fasterxml.jackson.module.mrbean.TestBridgeMethods.testGenericCovariantProperty(TestBridgeMethods.java:73)

Solution

Skip over all synthetic and bridge methods as is done in databind's AnnotatedMethodCollector.

@robbytx
Copy link
Contributor Author

robbytx commented Sep 17, 2020

My company's corporate CLA should be coming shortly -- they've agreed to sign it.

robbytx
robbytx commented Sep 17, 2020
View reviewed changes
mrbean/src/test/java/com/fasterxml/jackson/module/mrbean/TestBridgeMethods.java
* </ul>
*/
@SuppressWarnings("unchecked")
private static <T> Class<T> reorderBridgeMethodFirst(Class<T> clazz, final String methodName) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love feedback if there is an easier way of doing this. I'm not very familiar with ASM, but I couldn't find any simpler APIs that let me reorder definitions within the class file.

@robbytx
Copy link
Contributor Author

robbytx commented Sep 18, 2020

This PR should be update to merge from https://github.com/bazaarvoice/jackson-modules-base/tree/bridge-methods

@cowtowncoder
Copy link
Member

Looks good wrt fix; test looks impressive but unfortunately I can not really say whether there might be an easier way.
Would be happy to merge this since I got CCLA.

But one quick question first: wouldn't this make sense to merge against 2.12, or is there specific reason it's for master?
I can cherry-pick the fix without problem I think but just want to make sure this wasn't intentional first.

@robbytx
Copy link
Contributor Author

robbytx commented Sep 20, 2020 •
edited
Loading

Similar to #102, I would like like this bug fix to go into 2.12, and into a patch version of 2.11 if appropriate.

Please advise if you would like me to recreate this PR from the BV repo against master, 2.12, or 2.11.

@cowtowncoder
Copy link
Member

I think that as per my comments on #102, this too can go in 2.11.

@robbytx robbytx mentioned this pull request Sep 22, 2020
Closed
@robbytx
Copy link
Contributor Author

robbytx commented Sep 22, 2020

Closing this in favor of #110 which targets 2.11

@robbytx robbytx closed this Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
mrbean
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robbytx @cowtowncoder